Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Sidepane logic #8202

Merged
merged 37 commits into from
Dec 14, 2021
Merged

Refactor Sidepane logic #8202

merged 37 commits into from
Dec 14, 2021

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Oct 30, 2021

Refactoring goals:

  • Remove all view references fromSidePaneComponent
  • Instead of having an instance of SidePane in SidePaneComponent for updating the view, we should use binding
  • Remove unnecessary abstraction in SidePane like (separating components and visibleComponents)
  • Maybe refactor SidePaneComponent into view and ViewModel
  • and finally, fix View menu for sidebar option can be out of sync with sidebar state  #8115

Edit: I ended up redesigning the whole component so don't take the above goals as a grant

Added classes

  • SidePaneContainerView / SidePaneContainerViewModel: This is the replacement for SidePane, it has almost the same actions/methods (show, hide, toggle, moveUp, moveDown) but in private because they are not used from the outside, it has one public method sidePaneVisibleProperty(sidePaneType) in the view, it returns the visibility property for sidePaneType, it's public because it's the only property used from the outside (check menu).
  • SidePaneHeaderView: This class represent the header view that used to be created using SidePaneComponent#getHeader, this class controls the position/visibility of a SidePaneView using actions received in the constructor (moveUpCommand, moveDownCommand, closeCommand).
  • SidePaneView: This is a replacement for SidePaneComponent, it glues together the content of the pane with the header.
  • GroupsSidePaneView: This is a specialized implementation of SidePaneView with 2 differences, an afterOpening method and a special header GroupsSidePaneHeaderView because groups pane uses an extra button for toggling intersection union, I didn't put afterOpening in the base implementation because the other sidepanes (webSearch and OpenOffice) don't use it.
  • GroupsSidePaneHeaderView: This is a specialized implementation of SidePaneHeaderView with an extra button for toggling intersection union, it handles it internally.
  • WebSearchPaneView: I just copy-pasted WebSearchPane#createContentPane into its own class.

Notes

  • The visibility property of side panes is updated when visiblePanes list changes in SidePaneContainerViewModel.
  • I gave up on afterOpening and beforeClosing methods in SidePaneComponent because they are only adding and removing panes to/from preferencesService.getSidePanePreferences().visiblePanes(), preferences visible panes are now updated from SidePaneContainerViewModel in show() and hide(), the only side pane that does something beside that, is GroupsSidePaneView that's why we added afterOpening to it.
  • SidePaneType has the title, icon, and action of the side pane so it can be considered a model in the new design.
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository
Desktop.2021.10.31.-.23.42.59.02.mp4

.

@calixtus
Copy link
Member

Great Idea, already looked into the issue about the view menu, but had not yet found a working solution. The enum of components seems to be a bit overcomplicated and inflexible too...

- Add immutable info to SidePaneType enum
- Replace SidePaneComponent by SidePaneView which has a SidePaneHeaderView that contains all input handling
- Create SidePaneContainerView the replacement for SidePane
@HoussemNasri HoussemNasri changed the title Refactor SidePane logic Refactor Sidepane logic Oct 31, 2021
- Fix #8115
- Deprecate the old implementation of Sidepane and remove all of its usage in the rest of the application
- Implement the toggle method in SidePaneContainerView
@HoussemNasri
Copy link
Member Author

Codacy is complaining and saying that there is a missing enum branch in the switch statement which is not true, can someone tell me what's wrong?

@HoussemNasri HoussemNasri marked this pull request as ready for review October 31, 2021 22:32
@calixtus
Copy link
Member

Forget about codacy!

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for you work! This is sooooo much better already.

I have a couple of further suggestions mainly around the main direction of information flow (using the StateManager class) and using javafx bindings.

- Swapping elements in ObservableLists is rather difficult to handle. The problem with this code here is that two events are fired (one for removing, and on for adding). Perhaps the easiest solution is to use the sort method using a custom comparator. Since the list is small we can also be inefficient and do this as follows: create a copy of visiblePanes, swap the elements there and then use this new list as a reference for the comparator
@calixtus
Copy link
Member

calixtus commented Nov 6, 2021

Is this PR ready for final review now?

@calixtus
Copy link
Member

calixtus commented Dec 6, 2021

Hi @HoussemNasri : since this PR was a bit stalled, I took the liberty to work a bit on it. After all the great work you already did, I just refactored SidePane and SidePaneViewModel a bit to better implement the MVVM-Pattern. Also I reduced the code duplication with the visible properties for the menu. Works with Bindings and only one source of truth now.

The second thing I tried to work on was the bug about the not-closing sidepane, when every component is closed. This should work now (I moved the listener from the visible property to the children list of the sidepane), but the issue mentioned by @Siedlerchr (#8082 (comment)) still remains, which means that the initial width of the sidepane if open on startup is not right. Need to investigate some more.

@calixtus
Copy link
Member

calixtus commented Dec 11, 2021

Fixes #8115
Fixes #8082

@calixtus
Copy link
Member

This should work now and close two bugs.
Big thanks to @HoussemNasri for refactoring the sidepane logic, which is way more clearer and understandable now.
Sorry it took so long to work on it, I had too much work in the last weeks. Should be better now.

@calixtus calixtus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions ui labels Dec 11, 2021
@HoussemNasri
Copy link
Member Author

Thank you @calixtus! I'm glad I got to work with you and other brilliant developers on this project, learning tons about making software according to best practices. I feel like I have more and better skills than ever now, too! I'll definitely be getting involved more as time goes on; both with Jabref and in the open-source community - so thank you again!

@Siedlerchr
Copy link
Member

@tobiasdiez Do you have any issues left? Otherwise I would like to merge this so we can get this in our next release asap

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have time to look at this in detail. If you think its fine, please go ahead and merge it.

@Siedlerchr Siedlerchr merged commit 851fa4b into JabRef:main Dec 14, 2021
@HoussemNasri HoussemNasri deleted the refactor-sidepane branch December 14, 2021 13:56
Siedlerchr added a commit that referenced this pull request Dec 17, 2021
* upstream/main:
  Don't register any database changes to the indexer while dropping a file (#8334)
  Fix ACM fetcher (#8338)
  Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5
  Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247)
  Refactor Sidepane logic (#8202)
  New translations JabRef_en.properties (Japanese) (#8331)
  Bump bcprov-jdk15on from 1.69 to 1.70 (#8333)
  Update Controlsfx to 11.1.1 (#8330)
  Update citeproc (#8329)
  Bump classgraph from 4.8.137 to 4.8.138 (#8327)
  Bump junit-platform-launcher from 1.8.1 to 1.8.2 (#8326)
  Remove outdated gradle deps check (#8324)
  Bump junit-jupiter from 5.8.1 to 5.8.2 (#8325)
  Bump libreoffice from 7.2.2 to 7.2.3 (#8328)
  Remove outdated ignores
Siedlerchr added a commit that referenced this pull request Dec 20, 2021
* upstream/main: (46 commits)
  New Crowdin updates (#8349)
  Bump pdfbox from 2.0.24 to 2.0.25 (#8345)
  Bump fontbox from 2.0.24 to 2.0.25 (#8348)
  Bump xmlunit-core from 2.8.3 to 2.8.4 (#8347)
  Bump tika-core from 2.1.0 to 2.2.0 (#8346)
  Added missing executable bindings to various commands (#8342)
  Update Gradle Wrapper from 7.3.1 to 7.3.2. (#8343)
  Fix issues with writing metadata to pdfs (#8332)
  add tinylog test (#8339)
  Tinylog (#8226)
  Don't register any database changes to the indexer while dropping a file (#8334)
  Fix ACM fetcher (#8338)
  Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5
  Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247)
  Refactor Sidepane logic (#8202)
  New translations JabRef_en.properties (Japanese) (#8331)
  Bump bcprov-jdk15on from 1.69 to 1.70 (#8333)
  Update Controlsfx to 11.1.1 (#8330)
  Update citeproc (#8329)
  Bump classgraph from 4.8.137 to 4.8.138 (#8327)
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View menu for sidebar option can be out of sync with sidebar state
4 participants